Prefer symbols over strings as indeterminants #4158
Conversation
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
c21d8a0
to
59b30e5
Compare
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
8acda26
to
1f14c6e
Compare
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
1f14c6e
to
4799631
Compare
|
@benlorenz is this failure something similar to what you work around in #4153 or is a genuine failure? alternative inputs: Test Failed at /Users/aaruni/Desktop/oscar-runners/runner-1/_work/Oscar.jl/Oscar.jl/test/PolyhedralGeometry/subdivision_of_points.jl:17
Expression: collect(maximal_cells(square_by_incidence)) == collect(maximal_cells(square_by_weights))
Evaluated: [[1, 2, 3], [2, 3, 4]] == [[2, 3, 4], [1, 2, 3]] |
| @@ -61,7 +61,7 @@ function global_tate_model(base::NormalToricVariety, | |||
| vs2 = collect(keys(defining_section_parametrization)) | |||
| @req all(in(["a1", "a2", "a3", "a4", "a6"]), vs2) "Only the Tate sections a1, a2, a3, a4, a6 must be parametrized" | |||
|
|
|||
| gens_base_names = [string(g) for g in gens(cox_ring(base))] | |||
| gens_base_names = [string(g) for g in symbols(cox_ring(base))] | |||
There was a problem hiding this comment.
@HereAround pinging you to make you aware that symbols(::MPolyRing) exists. In many of these cases you probably don't even need to convert to strings afterwards, but I kept it like this if I didn't see an obvious way to change the below code
I only noticed (and now fixed) those in |
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
8d55dad
to
15b155b
Compare
| @test collect(maximal_cells(square_by_incidence)) == | ||
| collect(maximal_cells(square_by_weights)) | ||
| @test issetequal(collect(maximal_cells(square_by_incidence)), | ||
| collect(maximal_cells(square_by_weights))) |
There was a problem hiding this comment.
@benlorenz I added this already here to try to get CI green eventually
There was a problem hiding this comment.
Ok, thanks! The collect should be unnecessary now but I can take care of that later.
For the cases in polyhedron.jl a rerun usually fixed it.
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
2 times, most recently
from
0784fd0
to
ef891b7
Compare
Co-authored-by: Max Horn <[email protected]>
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
ef891b7
to
fdbbec1
Compare
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
fdbbec1
to
6daf07b
Compare
There was a problem hiding this comment.
The Algebraic Geometry/Commutative Algebra part looks good provided tests pass -- with the exception that I did not look at toric stuff (because you already pinged @HereAround ) and IntersectionTheory should be pinged as well.
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
6daf07b
to
f3d075e
Compare
lgoettgens
added
the
optimization
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4158 +/- ##
==========================================
- Coverage 84.70% 84.70% -0.01%
==========================================
Files 628 628
Lines 84449 84447 -2
==========================================
- Hits 71529 71527 -2
Misses 12920 12920
|
polynomial_ringused to get discarded and a line latergenswas called@lkastner @HereAround could you have a look if the attributes(moved to #4159)coordinate_namesandcoordinate_names_of_torusin the toric varieties could be changed toVector{Symbol}instead ofVector{String}to reduce the allocations from copying this around and passing it topolynomial_ringetc?